-
Notifications
You must be signed in to change notification settings - Fork 1.2k
change: When rootlessDocker is enabled, return a fixed SageMaker IP #5236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/sagemaker/local/utils.py
Outdated
"RootlessDocker not detected, falling back to remote host IP or localhost." | ||
) | ||
except subprocess.SubprocessError: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this log or return the exception ? Passing will be a confusing experience to the users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I have updated my PR to log the exception.
# log the result of check | ||
logger.warning("RootlessDocker detected (Cgroup Driver: none), returning fixed IP.") | ||
# SageMaker rootless Docker detected - return fixed IP | ||
return "172.17.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a general IP address used throughout docker ?
Where are we getting this value from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IP is from the LLC document. This is supposed to be configured by SageMaker. Sravya told me that we will use this specific IP.
https://tiny.amazon.com/eulnovpq/motivation
Can you add more details on how you tested this? For example, how did you spin up docker container with and without rootless mode enabled? How did you try to access the ports? Was all the testing done on JL? Did you test locally? |
if process.returncode == 0: # Check return code instead of stderr | ||
output_text = output.decode("utf-8") | ||
# Check for rootless Docker by looking at Cgroup Driver | ||
if "Cgroup Driver: none" in output_text: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this value for rootful docker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For rootful docker right now it is cgroupfs
.
Can you also verify this in local mode before merging? |
Issue #, if available:
N/A - Internal requirement for SageMaker Studio rootless Docker support
Description of changes:
Enhanced
get_docker_host()
function insrc/sagemaker/local/utils.py
to support rootless Docker environments:Cgroup Driver: none
indocker info
output172.17.0.1
for rootless Docker instead oflocalhost
Testing done:
test_get_docker_host_rootless_docker
)test_get_docker_host_traditional_docker
)We then launch JupyterLab App in 1). a domain without rootlessDocker and 2). a domain with enabled rootlessDocker. And then we create and upload pysdk wheel artifact and install it in the JupyterLab App terminal. We run
python -c "from sagemaker.local.utils import get_docker_host; print(get_docker_host())"
localhost
as expected.172.17.0.1
.Ref: https://quip-amazon.com/EvbfAyrac6Nh/SageMaker-Studio-V2-Docker-access-in-VPCOnly-mode#temp:C:bDf835bbc571f5245dfb5e32037b
Merge Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
unique_name_from_base
to create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.